Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: tm2 clean up #825

Closed
wants to merge 3 commits into from
Closed

WIP: tm2 clean up #825

wants to merge 3 commits into from

Conversation

piux2
Copy link
Contributor

@piux2 piux2 commented May 16, 2023

Description

Part of #692 #585

BREAKING CHANGE: In GitHub Action Flow, the build script needs to be updated on moving tm2txsync from tm2/cmd to gno.land/cmd

gno.land/

It provides information and tools necessary for running gno.land node. It includes

  • a node application for starting gnoland nodes to deliver an RPC endpoint
  • gnoweb for web access to the chain.
  • tools to interact with gno nodes.
  • a cosmos SDK gno VM module with keeper and handler.

In the future, we plan to consolidate tools for node deployment and monitoring. The future plan also includes supporting both IBC and ICS.

gnovm/

Gnovm is agnostic to any SDK or chain.

It is intended for gno contract developers and gno VM developers. It includes

  • a gno development CLI
  • gnolang language
  • virtual machine implementation.
  • standard libraries for gno contracts,
  • examples and test cases.

Future plans include making gnovm compatible with IBC

tm2/

Tendermint2 is a BFT consensus engine and a set of tools for blockchain builders

More details can be found at
https://github.com/tendermint/tendermint2#readme

In the future, tm2 plans to:

  • Remain synchronized with the tendermint2 repository.
  • Port issue fixes between Tendermint v0.23.4 and v0.34.
  • Ensure compatibility with IBC and ABCI2.

The dependences among tm2, gnovm and gno.land

  • gno.land depends on tm2 and gnovm
  • gnovm depends on tm2
  • tm2 is independent to gnovm and gno.land
flowchart LR
    A[gno.land]--> B[gnovm] --> C[tm2]
    A --> C

Loading

Discussion

client commands

We aim to establish a clear dependency structure between these components as outlined earlier.

Ideally we'd like to have clear dependency between these components as listed above

At present, tm2/pkg/crypto/keys/client/root.go is dependent on gno.land/pkg/sdk/vm.
We're considering moving the following commands to gno.land/cmd/gnokey/ and eliminating the newMakeTxCmd() from cmd.AddSubCommands() in tm2/pkg/crypto/client/root.go:

  • addpkg.go
  • call.go
  • send.go
  • maketx.go

However, these files rely on the private config structure and private methods defined in tm2/pkg/crypto/keys/client/. Until we decide the next steps, these files will remain in tm2, which means that tm2/ continues to depend on gno.lang/ for now.

genproto

As for the genproto, we need to decide where misc/genproto/genproto.go should be placed.

sample tendermint2 program

Since we've moved out applications that depend on gno in tm2, we need to consider whether we should reintroduce the sample tendermint application or create a new one.

tendermint classic main.go

Contributors Checklist

  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests

Maintainers Checklist

  • Checked that the author followed the guidelines in CONTRIBUTING.md
  • Checked the conventional-commit (especially PR title and verb, presence of BREAKING CHANGE: in the body)
  • Ensured that this PR is not a significant change or confirmed that the review/consideration process was appropriate for the change

@piux2 piux2 requested review from a team, jaekwon and moul as code owners May 16, 2023 03:22
@piux2
Copy link
Contributor Author

piux2 commented May 16, 2023

The action flow failed during CI. Once this is approved and merged. We might need to move tm2txsync in the matrix.program from .github/workflows/tm2.yml to .github/workflows/gnoland.yml

@zivkovicmilos zivkovicmilos self-requested a review May 16, 2023 21:35
@ajnavarro ajnavarro added 📦 🤖 gnovm Issues or PRs gnovm related 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related 🌟 improvement performance improvements, refactors ... labels May 18, 2023
@jaekwon
Copy link
Contributor

jaekwon commented May 19, 2023

can the vm package move into gnoland instead?
it could go in gnovm, but maybe it's better to leave gnovm agnostic to any SDK/chain.

@github-actions github-actions bot removed the 📦 🤖 gnovm Issues or PRs gnovm related label May 22, 2023
@piux2
Copy link
Contributor Author

piux2 commented May 22, 2023

can the vm package move into gnoland instead? it could go in gnovm, but maybe it's better to leave gnovm agnostic to any SDK/chain.

Agree; I should have done that.

Currently, tm2/pkg/crypto/keys/client/call.go and client/addpkg.go depend on the vm sdk module to construct the MsgCall and MsgAddPkg. When we transfer call.go and addpkg.go from tm2 to gno.land, everything should be in place. I have moved the pkg/sdk/vm folder from gnovm to gno.land. As of now, tm2 relies on gno.land instead of gnovm.

@piux2 piux2 added the 📦 🤖 gnovm Issues or PRs gnovm related label May 24, 2023
@@ -6,12 +6,13 @@ help:
rundep=go run -modfile ../misc/devdeps/go.mod

.PHONY: build
build: build.gnoland build.gnokey build.gnoweb build.gnofaucet
build: build.gnoland build.gnokey build.gnoweb build.gnofaucet _build.tm2txsync
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
build: build.gnoland build.gnokey build.gnoweb build.gnofaucet _build.tm2txsync
build: build.gnoland build.gnokey build.gnoweb build.gnofaucet _build.gnotxsync

Should be renamed to gnotxsync too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. I updated them all.

@github-actions github-actions bot removed the 📦 🤖 gnovm Issues or PRs gnovm related label May 29, 2023
@moul
Copy link
Member

moul commented Jul 31, 2023

Hey @piux2, what's your plan for this initiative?

  1. Should we work on fixing conflicts in your current work or start from scratch?
  2. Is it better to merge something useful now and refine it later in future PRs?
  3. Need any help? Just let us know! 😊

Comment on lines +9 to +15
build: build.gnoland build.gnokey build.gnoweb build.gnofaucet _build.gnotxsync

build.gnoland:; go build -o build/gnoland ./cmd/gnoland
build.gnoweb:; go build -o build/gnoweb ./cmd/gnoweb
build.gnofaucet:; go build -o build/gnofaucet ./cmd/gnofaucet
build.gnokey:; go build -o build/gnokey ./cmd/gnokey
_build.gnotxsync:; go build -o build/gnotxsync ./cmd/gnotxsync
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
build: build.gnoland build.gnokey build.gnoweb build.gnofaucet _build.gnotxsync
build.gnoland:; go build -o build/gnoland ./cmd/gnoland
build.gnoweb:; go build -o build/gnoweb ./cmd/gnoweb
build.gnofaucet:; go build -o build/gnofaucet ./cmd/gnofaucet
build.gnokey:; go build -o build/gnokey ./cmd/gnokey
_build.gnotxsync:; go build -o build/gnotxsync ./cmd/gnotxsync
build: build.gnoland build.gnokey build.gnoweb build.gnofaucet build.gnotxsync
build.gnoland:; go build -o build/gnoland ./cmd/gnoland
build.gnoweb:; go build -o build/gnoweb ./cmd/gnoweb
build.gnofaucet:; go build -o build/gnofaucet ./cmd/gnofaucet
build.gnokey:; go build -o build/gnokey ./cmd/gnokey
build.gnotxsync:; go build -o build/gnotxsync ./cmd/gnotxsync

@@ -7,12 +7,12 @@ import (
"flag"
"fmt"

"github.com/gnolang/gno/gno.land/pkg/sdk/vm"
Copy link
Member

@moul moul Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addpkg should be defined in gno.land or gnovm, not tm2.

Copy link
Contributor

@tbruyelle tbruyelle Aug 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears there's many shared struct between the different commands, so moving only call.go and addpkg.go in gno.land/cmd/gnokey is not straightforward.

What about moving the whole tm2/pkg/crypto/keys/client folder into gno.land/cmd/gnokey/client ? I tested and it requires only a few changes in some imports:

-	"github.com/gnolang/gno/tm2/pkg/crypto/keys/client"
+	"github.com/gnolang/gno/gno.land/cmd/gnokey/client"

tm2/pkg/crypto/keys/client is not used at all inside tm2.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's a smart approach: let's start by moving to gno.land and then take the time to design well-crafted reusable components in the right place for future blockchains.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have the right to push to this repo, but I can provide the patch, even if the change is pretty straighforward.

Copy link
Contributor Author

@piux2 piux2 Aug 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tm2/pkg/crypto/keys/client is part of the generic tm2 tool set. We should not remove it from the tm2 folder. The purpose of the separation is that people can create other tools using tm2 packages in addition to gnokey

Copy link
Contributor

@tbruyelle tbruyelle Aug 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really generic if I may: one of the first thing it does is fetching the config from $GNO_HOME or $XDG_CONFIG_HOME/gno.

But I understand your point, it would be better to add the addpkg and call subcommands in the gnokey package. I just feel it's a shame to keep this ultimate dependency.

The other way maybe is to simply turn public the related structs (makeTxCfg, queryCfg) and methods (signAndBroadcast, queryHandler, ...) . At some point, to make this package more flexible, that makes sense.

@@ -5,11 +5,11 @@ import (
"flag"
"fmt"

"github.com/gnolang/gno/gno.land/pkg/sdk/vm"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for call, should be defined in gno.land on gnovm, but not tm2.

@moul
Copy link
Member

moul commented Jul 31, 2023

maketx and send stay in tm2; addpkg and call move to gnovm; and gno.land includes all 4 commands.

Alternatively, we can keep maketx and send in tm2, move addpkg and call to gno.land, and later move them to gnovm while ensuring their generic functionality.

@piux2
Copy link
Contributor Author

piux2 commented Aug 1, 2023

As I mentioned in the top discussion section

We're considering moving the following commands to gno.land/cmd/gnokey/ and eliminating the newMakeTxCmd() from cmd.AddSubCommands() in tm2/pkg/crypto/client/root.go:

addpkg.go
call.go
send.go
maketx.go
However, these files rely on the private config structure and private methods defined in tm2/pkg/crypto/keys/client/. Until we decide the next steps, these files will remain in tm2, which means that tm2/ continues to depend on gno.lang/ for now.

We can only move them out and create separation if we do either one of the below.

  1. we make the dependent config struct as public structure and sub-command methods as public.
  2. we create another abstraction layer on these dependent private structures and methods.

Please checkout the tm2/pkg/crypto/client/root.go and the cfg variable in each command.

@moul
Copy link
Member

moul commented Aug 3, 2023

Apologies for the misunderstanding. Let's update this PR to be current and conflict-free, with passing CI.

In another PR, we can explore an efficient method to finish the actual split tm2<>gnovm. Thank you!

@jaekwon
Copy link
Contributor

jaekwon commented Aug 9, 2023

should i take a look at this now?

@moul
Copy link
Member

moul commented Aug 10, 2023

Jae, no need to check this immediately. But feel free.

While I initially thought this PR would finalize the split, it's a preliminary step. I suggest we help/let @piux2 to pass CI and merge conflicts. Then, I'll review recent changes (you pre-approved a long time ago), then merge.

Later, we'll seek your review for the new complementary PR.

@thehowl
Copy link
Member

thehowl commented Aug 30, 2023

Superseded by #1080

@thehowl thehowl closed this Aug 30, 2023
moul added a commit that referenced this pull request Aug 31, 2023
<!-- please provide a detailed description of the changes made in this
pull request. -->

<details><summary>Original text from #825</summary>
<p>

# Description
Part of #692 #585 

BREAKING CHANGE: In GitHub Action Flow, the build script needs to be
updated on moving tm2txsync from tm2/cmd to gno.land/cmd

## gno.land/ 

It provides information and tools necessary for running gno.land node.
It includes
- a node application for starting gnoland nodes to deliver an RPC
endpoint
- gnoweb for web access to the chain. 
- tools to interact with gno nodes.
- a cosmos SDK gno VM module with keeper and handler.

In the future, we plan to consolidate tools for node deployment and
monitoring. The future plan also includes supporting both IBC and ICS.

## gnovm/ 

Gnovm is agnostic to any SDK or chain.

It is intended for gno contract developers and gno VM developers. It
includes
- a gno development CLI
- gnolang language
- virtual machine implementation.
- standard libraries for gno contracts, 
- examples and test cases.

Future plans include making gnovm compatible with IBC

## tm2/ 

Tendermint2 is a BFT consensus engine and a set of tools for blockchain
builders

More details can be found at 

[https://github.com/tendermint/tendermint2#readme](https://github.com/tendermint/tendermint2#readme)


In the future, tm2 plans to:

- Remain synchronized with the tendermint2 repository.
- Port issue fixes between Tendermint v0.23.4 and v0.34.
- Ensure compatibility with IBC and ABCI2.

# The dependences among tm2, gnovm and gno.land

- gno.land depends on tm2 and gnovm
- gnovm depends on tm2 
- tm2 is independent to gnovm and gno.land

```mermaid

flowchart LR
    A[gno.land]--> B[gnovm] --> C[tm2]
    A --> C

```

# Discussion

## client commands 

We aim to establish a clear dependency structure between these
components as outlined earlier.

Ideally we'd like to have clear dependency between these components as
listed above

At present, tm2/pkg/crypto/keys/client/root.go is dependent on
gno.land/pkg/sdk/vm.
We're considering moving the following commands to gno.land/cmd/gnokey/
and eliminating the newMakeTxCmd() from cmd.AddSubCommands() in
tm2/pkg/crypto/client/root.go:

- addpkg.go
- call.go
- send.go
- maketx.go 

However, these files rely on the private config structure and private
methods defined in tm2/pkg/crypto/keys/client/. Until we decide the next
steps, these files will remain in tm2, which means that tm2/ continues
to depend on gno.lang/ for now.

## genproto

As for the genproto, we need to decide where misc/genproto/genproto.go
should be placed.

## sample tendermint2 program

Since we've moved out applications that depend on gno in tm2, we need to
consider whether we should reintroduce the sample tendermint application
or create a new one.

[tendermint classic
main.go](https://github.com/tendermint/classic/tree/master/cmd/tendermint)

</p>
</details> 

<details><summary>Contributors' checklist...</summary>

- [ ] Added new tests, or not needed, or not feasible
- [ ] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [ ] Updated the official documentation or not needed
- [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [ ] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Co-authored-by: piux2 <>
Co-authored-by: Manfred Touron <[email protected]>
@moul moul added this to the 🚀 main.gno.land (required) milestone Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related 🌟 improvement performance improvements, refactors ...
Projects
Status: Done
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants